feat: add max_page to chunk_by_title and remove multipage_sections#4382
feat: add max_page to chunk_by_title and remove multipage_sections#4382issahammoud wants to merge 8 commits into
Conversation
… chunking Introduces a new optional `max_page: int` argument to `chunk_by_title()`. When set, a chunk is closed whenever an element's page number exceeds the chunk's start page by `max_page` or more, giving callers a hard upper bound on how many pages a single chunk may span regardless of character/token limits. Implementation adds `is_on_page_exceeding_max(max_page)` to `base.py` as a stateful boundary-predicate factory (same pattern as `is_on_next_page()`). `Title` elements reset the predicate's page counter without firing, so each title-delimited section's page span is measured independently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 5 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
PreChunkCombiner only checked size constraints (combine_text_under_n_chars, hard_max) and table isolation when deciding whether to merge consecutive pre-chunks. This meant two pre-chunks split by the is_on_page_exceeding_max predicate could be silently re-merged if their combined text was small enough, violating the advertised max_page hard limit. Fix: - Add max_page hook to ChunkingOptions base (returns None; overridden by _ByTitleChunkingOptions to read the caller's kwarg). This lets PreChunk.can_combine() check the limit without knowing the strategy. - Add PreChunk._page_span cached property returning (first_page, last_page) for elements that carry a page number, or None when none do. - Guard PreChunk.can_combine() with a page-span check: block combination whenever the combined span would exceed max_page pages. - Add regression test that exercises the combiner path directly (does not suppress combining via combine_text_under_n_chars=0). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for catching this — the issue is valid. Root cause: Fix (commit 39b24c2):
|
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Shadow auto-approve: would require human review. Adds a new max_page parameter to chunking logic, modifying core PreChunk combination and boundary detection. While well-tested, this feature introduces new stateful predicates and changes to core chunking paths, which carries moderate risk.
Re-trigger cubic
multipage_sections=False is exactly max_page=1 — both break a chunk on every page change. Keeping two knobs for the same thing is confusing, so max_page now subsumes multipage_sections entirely. Changes: - chunk_by_title() emits DeprecationWarning when multipage_sections is passed; callers should migrate to max_page=1. - _ByTitleChunkingOptions.max_page translates multipage_sections=False → 1 for backward compat, so existing call sites keep working without code changes. - boundary_predicates no longer uses is_on_next_page(); is_on_page_exceeding_max handles all page-boundary cases including max_page=1. - Passing both multipage_sections and max_page raises ValueError (they conflict). - multipage_sections property is kept as a read-only backward-compat alias: returns False when max_page==1, True otherwise. - dispatch.py help text updated to advertise max_page and mark multipage_sections as deprecated. - Tests updated: existing multipage_sections call sites wrapped in pytest.warns(DeprecationWarning); new tests cover translation, conflict detection, warning emission, and max_page=1 / multipage_sections=False equivalence. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Two issues flagged by code review: 1. dispatch.py: max_page appeared as a generic parameter but is only implemented for the by_title strategy. Added "Only applies to 'by_title' strategy." caveat, matching the existing note on combine_text_under_n_chars. 2. title.py: the DeprecationWarning only told multipage_sections=False users to migrate (max_page=1), leaving multipage_sections=True users with no clear guidance. Updated the message to cover both cases: max_page=1 for False, omit max_page entirely for True. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would require human review. This PR changes core chunking logic that could affect RAG output quality and correctness. Because of the complexity and risk of regression, it warrants human review.
Re-trigger cubic
… exclusively chunk_by_title with max_page=None is the general form of the old chunk_by_title (formerly multipage_sections=True), and max_page=1 replaces multipage_sections=False. The two parameters were redundant — max_page is a strict superset. Changes: - Remove multipage_sections parameter from chunk_by_title() entirely. - Remove translation logic, backward-compat property, and conflict validation from _ByTitleChunkingOptions; max_page is now the single source of truth. - Remove is_on_next_page() usage; is_on_page_exceeding_max handles all page boundary cases including max_page=1. - Remove deprecation warning infrastructure (import warnings, warning block). - Remove multipage_sections entry from dispatch.py docstring injection. - Update base.py comment to reference max_page instead of multipage_sections. - Update all tests: replace multipage_sections=False with max_page=1, drop multipage_sections=True (it was the default), remove the five deprecation- related unit tests and the equivalence integration test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ameter Completely removing multipage_sections was a breaking change. Instead, keep it in the signature with a DeprecationWarning so existing callers continue to work until a future major release. Behaviour: - Passing multipage_sections emits DeprecationWarning pointing to max_page. - multipage_sections=False translates to max_page=1 when max_page is not set. - multipage_sections=True is a no-op (matches the default max_page=None). - When both multipage_sections and max_page are provided, max_page wins silently (no ValueError). Tests updated to wrap multipage_sections call sites with pytest.warns(DeprecationWarning) and replace the old conflict-raises test with a max_page-priority test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="unstructured/chunking/title.py">
<violation number="1" location="unstructured/chunking/title.py:31">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
PR description claims multipage_sections is 'removed entirely' and 'passing it now raises TypeError', but the code adds it back as an optional parameter with a DeprecationWarning and backward-compatibility logic instead.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| max_characters: Optional[int] = None, | ||
| max_page: Optional[int] = None, | ||
| max_tokens: Optional[int] = None, | ||
| multipage_sections: Optional[bool] = None, |
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
PR description claims multipage_sections is 'removed entirely' and 'passing it now raises TypeError', but the code adds it back as an optional parameter with a DeprecationWarning and backward-compatibility logic instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unstructured/chunking/title.py, line 31:
<comment>PR description claims multipage_sections is 'removed entirely' and 'passing it now raises TypeError', but the code adds it back as an optional parameter with a DeprecationWarning and backward-compatibility logic instead.</comment>
<file context>
@@ -27,6 +28,7 @@ def chunk_by_title(
max_characters: Optional[int] = None,
max_page: Optional[int] = None,
max_tokens: Optional[int] = None,
+ multipage_sections: Optional[bool] = None,
new_after_n_chars: Optional[int] = None,
new_after_n_tokens: Optional[int] = None,
</file context>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Summary
Adds a
max_pageparameter tochunk_by_title()that gives callers a hard upper bound on how many pages a single chunk may span.max_pageis a strict superset of the existingmultipage_sectionsflag:max_page=None(default) matchesmultipage_sections=True, andmax_page=1matchesmultipage_sections=False— plus the new ability to express any page-count limit (max_page=2,max_page=3, …) that the old binary flag could not.multipage_sectionsis kept for backward compatibility but deprecated. It emits aDeprecationWarningand will be removed in a future release.Motivation
chunk_by_titlesupported character/token-based size limits and an all-or-nothing page boundary toggle (multipage_sections). There was no way to say "allow cross-page sections, but cap each chunk at N pages". This is a common need in RAG pipelines where page-level context windows matter — e.g. a retriever that attaches page images to each chunk should not pull more than a fixed number of page images per chunk.Behaviour
When
max_page=Nis specified, the chunker tracks the page where each chunk began and starts a new chunk as soon as an element's page number isNor more pages past that start page:by_titleboundaries (title, size, token).Titleelement resets the page counter without double-firing, so each title-delimited section's page span is measured independently.PreChunk.can_combine()also enforcesmax_page, soPreChunkCombinercannot silently re-merge pre-chunks that were split by the page boundary.metadata.page_numberare assumed to continue the current page.max_page < 1raisesValueError.multipage_sectionsdeprecationmultipage_sectionsis kept but deprecated. Existing call sites continue to work without modification; they will receive aDeprecationWarningpointing to themax_pageequivalent.multipage_sections=True(default)max_page(default)multipage_sections=Falsemax_page=1When both
multipage_sectionsandmax_pageare provided,max_pagetakes priority silently — no error is raised.Example
Bug fix: PreChunkCombiner was undoing max_page boundaries
Root cause:
PreChunkercorrectly split elements usingis_on_page_exceeding_max, butPreChunkCombineronly checkedcombine_text_under_n_charsandhard_maxwhen deciding whether to merge consecutive pre-chunks. Two pre-chunks split by amax_pageboundary could be silently re-merged if their combined text was small enough, makingmax_pagea soft hint rather than a hard limit.Fix:
max_pagehook toChunkingOptionsbase (returnsNone; overridden by_ByTitleChunkingOptions) soPreChunk.can_combine()can enforce the limit without coupling to a specific strategy.PreChunk._page_span— a cached property returning(first_page, last_page)for elements that carry a page number.PreChunk.can_combine()to block combination whenever the merged span would exceedmax_pagepages.Changes
unstructured/chunking/base.pyis_on_page_exceeding_max(max_page)boundary predicate factory — handles all page-boundary cases includingmax_page=1.ChunkingOptions.max_page— new cached property returningNoneby default; overridden by_ByTitleChunkingOptions.PreChunk._page_span— cached property returning(first_page, last_page).PreChunk.can_combine()— page-span guard blocks combination that would violatemax_page.unstructured/chunking/title.pychunk_by_title()gainsmax_page: Optional[int] = None.multipage_sectionsis kept but emitsDeprecationWarningwhen passed._ByTitleChunkingOptions.max_page— readsmax_pagekwarg directly; falls back to translatingmultipage_sections=False→1whenmax_pageis not set.max_pagetakes priority when both are provided._ByTitleChunkingOptions.multipage_sections— kept as a read-only backward-compat alias (max_page != 1)._ByTitleChunkingOptions.boundary_predicates— usesis_titleplusis_on_page_exceeding_maxwhenmax_pageis set;is_on_next_page()removed._ByTitleChunkingOptions._validate()— rejectsmax_page < 1.unstructured/chunking/dispatch.pymax_page(by_title only) and markmultipage_sectionsas deprecated.Structural diff
%%{init: {'layout': 'elk', 'elk': {'direction': 'RIGHT'}, 'maxTextSize': 999999, 'theme': 'base', 'themeVariables': {'background': '#ffffff', 'clusterBkg': '#f8fafc', 'clusterBorder': '#94a3b8', 'primaryColor': '#f8fafc', 'primaryBorderColor': '#94a3b8', 'primaryTextColor': '#1e293b', 'lineColor': '#64748b', 'fontSize': '13px', 'fontFamily': 'ui-monospace, SFMono-Regular, Menlo, monospace'}}}%% classDiagram direction LR class n2["_ByTitleChunkingOptions"] { <<unstructured/chunking/title.py>> + max_page() ~ _validate() ~ boundary_predicates() ~ iter_boundary_predicates() ~ multipage_sections() } class n5["unstructured/chunking/title.py"] { ~ chunk_by_title() sig } class n0["ChunkingOptions"] { <<unstructured/chunking/base.py>> + max_page() } class n1["PreChunk"] { <<unstructured/chunking/base.py>> + _page_span() ~ can_combine() calls +1 } class n3["unstructured/chunking/base.py"] { + is_on_page_exceeding_max() + page_count_exceeded() } style n2 fill:#fefce8,color:#854d0e,stroke:#f59e0b,stroke-width:3px style n5 fill:#fefce8,color:#854d0e,stroke:#f59e0b,stroke-width:3px style n0 fill:#f0fdf4,color:#166534,stroke:#22c55e,stroke-width:3px style n1 fill:#fefce8,color:#854d0e,stroke:#f59e0b,stroke-width:3px style n3 fill:#f0fdf4,color:#166534,stroke:#22c55e,stroke-width:3px n2 --|> n0 n2 --> n3 : callsChunkingOptions.max_page,is_on_page_exceeding_max,page_count_exceeded)_ByTitleChunkingOptions,PreChunk,chunk_by_titlesignature,dispatch.py)_ByTitleChunkingOptions --|> ChunkingOptions— inherits and overridesmax_page_ByTitleChunkingOptions --> base.py— callsis_on_page_exceeding_max;is_on_next_pageremovedmultipage_sections()remains on_ByTitleChunkingOptionsas a read-only backward-compat aliasTesting
pytest test_unstructured/chunking/test_title.py -q # 60 passed, 7 skippedNew / updated tests:
test_max_page_1_is_equivalent_to_multipage_sections_false— provesmax_page=1and the deprecatedmultipage_sections=Falseproduce identical outputtest_chunk_by_title_respects_max_page— chunks split at correct page boundariestest_chunk_by_title_max_page_resets_on_title—Titleresets the counter independently per sectiontest_chunk_by_title_max_page_1_breaks_on_every_page—max_page=1breaks on every page changetest_chunk_by_title_max_page_not_undone_by_combiner— regression: combiner cannot re-merge max_page-split pre-chunkstest_chunk_by_title_max_page_none_does_not_add_page_boundary— omittingmax_pageleaves existing behaviour unchangedit_translates_multipage_sections_false_to_max_page_1— internal translation verifiedit_leaves_max_page_none_when_multipage_sections_is_true— no-op case verifiedit_prefers_max_page_over_multipage_sections_when_both_are_set— priority rule verifiedit_emits_deprecation_warning_when_multipage_sections_is_used— warning verifiedit_does_not_warn_when_multipage_sections_is_not_passed— no spurious warningsit_accepts_valid_max_page_values— parametrized: 1, 3, Noneit_rejects_max_page_less_than_one— parametrized: 0, -1, -10multipage_sectionsintegration tests wrapped withpytest.warns(DeprecationWarning)Checklist
60 passed, 7 skipped)CHANGELOG.mdupdated__version__.pybumped to0.23.4